Fix error case memory bug in GetHostName() #11544
Conversation
jowlyzhang
force-pushed
the
fix_error_msg
branch
from
cb35690
to
37a6a2c
Compare
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jowlyzhang
force-pushed
the
fix_error_msg
branch
from
37a6a2c
to
532de97
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks for the fix! I suggest a more clear and direct summary like "Fix error case memory bug in GetHostName()". In case you were taking inspiration from some of the soft language in my PR #11538, that is kind of a special case where it's hard to promise what is actually safe usage during static destruction and we nevertheless need to make reasonable accommodations to avoid bugs or crashes, as there might not be a clear path to guaranteed safe usage.
In this case, the old code was completely unsafe (on an error handling path).
Rant: It would be nice to deprecate and get rid of this API with its C-style calling conventions, but that's probably more work than it's worth. Back to the important things.
jowlyzhang
changed the title
Thanks Peter for the review and the suggested summary. You are right that this is not just a good-to-have improvement, but an absolutely necessary fix for an incorrectly handled error case. |
jowlyzhang
force-pushed
the
fix_error_msg
branch
from
532de97
to
25687c2
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 6649978. |
Fix the error handling in
GetHostNamefor non EFAULT, non EINVAL error. Current handling will cause stack overflow when non null-terminated c style string is inname, e.g. ENAMETOOLONG, when thenamebuffer is not big enough and the host name is truncated.Test Plan: